Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Rest Client] Support for Cookies + Fix secret headers + Collectible errors #2474

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

ajkauffmann
Copy link
Contributor

@ajkauffmann ajkauffmann commented Dec 9, 2024

Summary

Adding support for cookies to the Rest Client
Fixing an issue with secret headers
Adding collectible error behavior
Redesigning internal code for better maintainability and uptaking new AL features like the keyword this

Work Item(s)

Fixes #1479 #989

Fixes AB#560010

Upgraded the runtime to version 14.0 and extended ID range to accommodate newly introduced components.

Added a comprehensive error handling system using enums for different error scenarios and a dedicated builder for error creation.

Improved HttpClient functionality by allowing the creation and management of cookies within requests and responses, enhancing flexibility.

Refactored HTTP methods with improved response checking, ensuring errors are collected and processed correctly.

Fix handling of secret content headers.

Refactored code with new constructor functions.
Replaced multiple overloads of HTTP request methods with simplified versions, removing boolean success indicators. Enhanced error handling by adopting collectible errors, and streamlined method implementations. Emphasized cleaner error descriptions in unsuccessful responses to improve debugging and user experience. Adjustments potentially improve maintainability and error traceability.
…eadability and maintainability

- Add missing newline at the end of HttpContent.Codeunit.al
- Rename GetRestClientException method to GetExceptionCode in HttpResponseMessage.Codeunit.al
- Remove unused GetExceptionName method in HttpResponseMessage.Codeunit.al
- Remove unused GetExceptionCode and GetExceptionName methods in HttpResponseMessageImpl.Codeunit.al
- Remove unused ConnectionErr and RequestFailedErr labels in RestClientImpl.Codeunit.al
- Update RestClientImpl.Codeunit.al to handle error collection more efficiently
- Add new codeunit HttpExceptionTests.Codeunit.al to test different types of exceptions
- Add new test to RequestMessageTests.Codeunit.al to test adding secret headers
@ajkauffmann ajkauffmann requested a review from a team as a code owner December 9, 2024 09:03
@github-actions github-actions bot added AL: System Application From Fork Pull request is coming from a fork labels Dec 9, 2024
@JesperSchulz JesperSchulz added Linked Issue is linked to a Azure Boards work item Integration GitHub request for Integration area labels Dec 9, 2024
@github-actions github-actions bot added this to the Version 26.0 milestone Dec 9, 2024
@pri-kise
Copy link
Contributor

pri-kise commented Dec 10, 2024

@ajkauffmann do I understand the changes with this correctly that we're now able to use the instance of the HttpContent when we use the method Create.
e.g. the following code

HttpContent := HttpContent.Create('Dummy');

can now be simplified to

HttpContent.Create('Dummy');

@ajkauffmann
Copy link
Contributor Author

@ajkauffmann do I understand the changes with this correctly that we're now able to use the instance of the HttpContent when we use the method Create. e.g. the following code

HttpContent := HttpContent.Create('Dummy');

can now be simplified to

HttpContent.Create('Dummy');

It's actually the other way around. Since Create() returns the instance by means of this, it can be assigned to a variable or parameter. If assigned to the same variable, like HttpContent := HttpContent.Create('Dummy'); then there is technically no difference with just HttpContent.Create('Dummy');.

@JesperSchulz
Copy link
Contributor

Looks like this one already is ready for a review by the product group! Let me find a Rest Client expert, as I'm definitely not one of those 😝

JesperSchulz
JesperSchulz previously approved these changes Dec 10, 2024
Copy link
Contributor

@JesperSchulz JesperSchulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one. Looks fine to me!

JesperSchulz
JesperSchulz previously approved these changes Dec 10, 2024
JesperSchulz
JesperSchulz previously approved these changes Dec 10, 2024
Copy link
Contributor

@JesperSchulz JesperSchulz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it.

Copy link
Contributor

@onbuyuka onbuyuka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests calling external service (httpbin) have been causing us issues, when the service is down for any reason, our tests fail and block all the gates. We are working on HTTP mocking in the compiler scenarios, maybe we should wait until that's in place and refactor these tests?

"Request changes" is for Postman calls

@JesperSchulz
Copy link
Contributor

And this is why we don't run tests with external dependencies in our CI:

Error: Function TestWithUseResponseCookies Assert.AreEqual failed. Expected:<200> (Integer). Actual:<503> (Integer). The response status code should be 200.

Let's disable those, and push forward 🥳

@onbuyuka onbuyuka self-requested a review December 17, 2024 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AL: System Application From Fork Pull request is coming from a fork Integration GitHub request for Integration area Linked Issue is linked to a Azure Boards work item
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: HttpContent.SetHeader failing when secret header already exists
6 participants